This repository was archived by the owner on Apr 23, 2021. It is now read-only.
Fix the wrong computation of dynamic strides for lowering AllocOp to LLVM#338
Open
tungld wants to merge 5 commits intotensorflow:masterfrom
Open
Fix the wrong computation of dynamic strides for lowering AllocOp to LLVM#338tungld wants to merge 5 commits intotensorflow:masterfrom
tungld wants to merge 5 commits intotensorflow:masterfrom
Conversation
MSVC has trouble resolving the static 'printOptionValue' from the method on llvm::cl::opt/list. This change renames the static method to avoid this conflict. PiperOrigin-RevId: 286978351
joker-eph
force-pushed
the
master
branch
3 times, most recently
from
48dcae0 to
3722f03
Compare
ftynse
suggested changes
Dec 26, 2019
Contributor
ftynse
left a comment
ftynse
left a comment
There was a problem hiding this comment.
Thanks! There's one extra simplification that can be made to make the code cleaner.
| auto nStrides = strides.size(); | ||
| SmallVector<Value, 4> strideValues(nStrides, nullptr); | ||
| for (auto indexedStride : llvm::enumerate(llvm::reverse(strides))) { | ||
| for (auto indexedStride : llvm::enumerate(strides)) { |
Contributor
There was a problem hiding this comment.
Actually, enumerate is useless here, the body of the loop never accesses indexedStride.value(). Could you just rewrite the loop to iterate on index to remove the confusion that led to the bug in the first place?
Author
|
Thanks, @ftynse! I removed |
ftynse
approved these changes
Dec 28, 2019
ftynse
pushed a commit
to llvm/llvm-project
that referenced
this pull request
Dec 28, 2019
…cOp to LLVM Leftover change from before the MLIR merge, reviewed at accepted at tensorflow/mlir#338.
Contributor
|
Landed as llvm/llvm-project@e5957ac. Since MLIR moved to LLVM, please submit your following contributions through https://reviews.llvm.org. Thank you! @joker-eph could you please close? |
bondhugula
pushed a commit
to bondhugula/llvm-project
that referenced
this pull request
Jan 7, 2020
…cOp to LLVM Leftover change from before the MLIR merge, reviewed at accepted at tensorflow/mlir#338.
arichardson
pushed a commit
to arichardson/llvm-project
that referenced
this pull request
Jan 20, 2020
…cOp to LLVM Leftover change from before the MLIR merge, reviewed at accepted at tensorflow/mlir#338.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The computation of dynamic strides in lowering AllocOp was wrong.
Given a MemRefType with
sizes = [5, 10], it computedstrides = [5, 1], while the correct answer should bestrides = [10, 1].